fix(reporting): reconcile program_completion_days sign and dual definitions#2416
fix(reporting): reconcile program_completion_days sign and dual definitions#2416blarghmatey wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR resolves a metric correctness/consistency problem in the dbt warehouse by fixing the sign of program_completion_days at the source mart layer and removing a redundant, independently-derived definition in the reporting layer.
Changes:
- Corrected
marts__combined_program_enrollment_detail.program_completion_daysto compute certificate date minus first course start date (positive day counts). - Updated
program_enrollment_with_user_reportto sourceprogram_complete_daysdirectly from the mart’sprogram_completion_days, and removed the no-longer-neededfirst_courserun_start_on_dateaggregate from the report’scourses_detailCTE.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/ol_dbt/models/marts/combined/marts__combined_program_enrollment_detail.sql | Fixes the date_diff operand order so program_completion_days matches the documented metric semantics. |
| src/ol_dbt/models/reporting/program_enrollment_with_user_report.sql | Eliminates a duplicate metric derivation by selecting program_completion_days from the upstream mart and dropping the unused supporting aggregate. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
blarghmatey
force-pushed
the
worktree-program-completion-days-consolidate
branch
from
90c6157 to
f6e4e9b
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| combined_users.user_address_postal_code, combined_users2.user_address_postal_code | ||
| ) as user_address_postal_code | ||
| , case when courses_detail.capstone_sum > 0 then 'Y' else 'N' end as capstone_ind | ||
| , date_diff( | ||
| 'day' | ||
| , courses_detail.first_courserun_start_on_date | ||
| , cast(substring(enrollment_detail.programcertificate_created_on, 1, 10) as date) | ||
| ) as program_complete_days | ||
| , enrollment_detail.program_completion_days as program_complete_days | ||
| from enrollment_detail | ||
| left join combined_users | ||
| on enrollment_detail.platform_name = '{{ var("edxorg") }}' |
There was a problem hiding this comment.
Bug: The calculation for first_courserun_start_on_date now incorrectly includes unverified enrollments, which alters the program_completion_days metric by using an earlier start date for some users.
Severity: MEDIUM
Suggested Fix
The reporting model should revert to its previous logic for calculating first_courserun_start_on_date. Instead of consuming the field from the mart, re-implement the logic that filters for courserunenrollment_enrollment_mode = 'verified' when determining the minimum course run start date. This will restore the original definition of the program_completion_days metric.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: src/ol_dbt/models/reporting/program_enrollment_with_user_report.sql#L91-L97
Potential issue: The reporting model `program_enrollment_with_user_report.sql` now
sources the `first_courserun_start_on_date` from a mart model. The previous
implementation calculated this date using only 'verified' enrollments. The mart model,
however, calculates this date based on all enrollment types, including unverified ones.
This change in logic means that for any user with an unverified enrollment that started
before their first verified one, the `program_completion_days` metric will be calculated
from this earlier date, leading to a significant and unintended change in the report's
output.
Did we get this right? 👍 / 👎 to inform future reviews.
🔎 ol-dbt impact — column-level blast radius✅ No column-level downstream impact detected for the changed models. Posted by |
… reporting model marts__combined_program_enrollment_detail computed program_completion_days as start-minus-certificate (negative). program_enrollment_with_user_report independently re-derived the same metric as program_complete_days with the correct direction but re-parsed from the raw certificate timestamp string instead of consuming the mart's field, creating two competing definitions. Swap the date_diff operands in the mart to certificate-minus-start (positive, matching both models' documented description), and have the reporting model select enrollment_detail.program_completion_days directly instead of re-deriving it from a differently-filtered join. Part of #2375 (2026-07 dbt warehouse audit). Witan task: tk-reconcile-fifth-competing-metric-definition-prog-3160aa. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
blarghmatey
force-pushed
the
worktree-program-completion-days-consolidate
branch
from
20ca5b7 to
cdbc344
Compare
| , date_diff( | ||
| 'day' | ||
| , final_combined_programs.programcertificate_created_on_date | ||
| , final_combined_programs.first_courserun_start_on_date | ||
| , final_combined_programs.programcertificate_created_on_date | ||
| ) as program_completion_days |
There was a problem hiding this comment.
Bug: The direct call to date_diff() with swapped arguments will be correct for Trino (production) but will likely produce incorrect negative values in DuckDB (development/testing) due to differing implementations.
Severity: MEDIUM
Suggested Fix
Replace the native SQL call date_diff(...) with the dbt macro {{ date_diff(...) }}. The macro is designed to handle differences between database adapters like Trino and DuckDB, ensuring the calculation is consistent across all environments.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location:
src/ol_dbt/models/marts/combined/marts__combined_program_enrollment_detail.sql#L362-L366
Potential issue: The code was changed to use `date_diff('day',
first_courserun_start_on_date, programcertificate_created_on_date)` to calculate
`program_completion_days`. This direct call to the native SQL function, instead of the
provided `{{ date_diff(...) }}` dbt macro, will behave differently across database
systems. While this change corrects the calculation for the Trino production
environment, it will likely cause the calculation to be incorrect (producing a negative
value) in the DuckDB development environment, which has a different argument order for
its native `date_diff` function. This creates a divergence between development and
production environments for a core metric.
What
Two competing definitions of the same metric under near-identical names:
marts__combined_program_enrollment_detail.program_completion_dayscomputeddate_diff('day', certificate_date, course_start_date)— start minus certificate, negative. This is the same inverted-sign bug as Fix NULL-unsafe SCD2 change detection in dim_course_run and dim_product #2380's family, tracked separately as Make remaining tracking-log event facts incremental #2385 / fixed via fix(dbt): resolve 3 P0 identity/fan-out/sign bugs from warehouse audit #2400.program_enrollment_with_user_report.program_complete_daysindependently re-derived the same concept, correct direction (certificate minus start), but re-parsed the rawprogramcertificate_created_onstring and recomputedfirst_courserun_start_on_datefrom its own verified-enrollment join instead of consuming the mart's field.Fix
date_diffoperands in the mart soprogram_completion_daysis certificate-minus-start (positive), matching both models' documented description ("days between first course start and program completion").program_enrollment_with_user_reportnow selectsenrollment_detail.program_completion_daysdirectly instead of re-deriving it, and dropped the now-unusedfirst_courserun_start_on_dateaggregate fromcourses_detail.Part of #2375 (2026-07 dbt warehouse audit).
Closes #2387
🤖 Generated with Claude Code